-
-
Notifications
You must be signed in to change notification settings - Fork 872
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add LiveQuery module to SDK #1712
feat: Add LiveQuery module to SDK #1712
Conversation
Thanks for opening this pull request!
|
1d61c91
to
4728be4
Compare
@vazarkevych Could you give a description of the issues that are stopping this PR currently? And could you please do a rebase to resolve the conflicts? Maybe the latest workflow changes address some of the issues. If you have any questions regarding the new workflow, maybe @dplewis could give a hand... |
@vazarkevych I can give a hand, can you give me access to your fork? |
@dplewis I gave access you to my fork |
Hi, @dplewis, Just a kind follow-up regarding the status of the ParseLiveQuery. Do you have any updates on your side? |
@vazarkevych I fixed the merge conflict waiting for CI to pass |
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #1712 +/- ##
==========================================
- Coverage 78.17% 78.16% -0.01%
==========================================
Files 307 307
Lines 36803 36803
==========================================
- Hits 28770 28768 -2
- Misses 8033 8035 +2 ☔ View full report in Codecov by Sentry. |
@vazarkevych I got the live query project to build successfully, looks like the release build is broken |
@vazarkevych I got the release working but ParseLiveQuery-OSX is broken. I don't know why BoltsSwift is compiling to macOS 10.10 in the CI and locally for me.
|
@vazarkevych @mtrezza The project builds , the examples compile and run, and the CI is working. I'm not familiar with how SPM works, can you check it? |
@vazarkevych Could you look into the remaining issues so we can get this merged? |
Hi @dplewis, thank you for your contribution. I just checked SPM, and it stopped working |
@vazarkevych I don't think I changed anything SPM related. |
09214d8
to
2eea533
Compare
@vazarkevych @mtrezza I got this to work via SPM. I added ParseObjC and ParseLiveQuery to my project from your github url and addParseLiveQuery branch. Then used
What stopped working? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation, README, code coverage, starter projects, demo, and tests can be done in separate PRs. Also there are still PR's left on the LiveQuery repo that need to be addressed
Amazing! Maybe we could at least copy/paste the LiveQuery README to this README under a new chapter? Or is that content now outdated because of the migration? |
That documentation is outdated and very hard to get working for new users, I can't get it working. Once the project is migrated the LiveQuery code needs some TLC |
Got it, then let's just wait for a review by @vazarkevych to confirm all is working as expected and then merge. |
Anyone could try this out and confirm the LiveQuery module is working? @parse-community/ios-sdk @extnous @HackShitUp |
Package.swift
Outdated
@@ -10,19 +10,22 @@ let package = Package( | |||
.tvOS(.v12), | |||
.watchOS(.v2)], | |||
products: [ | |||
.library(name: "ParseObjC", targets: ["ParseCore"]), | |||
.library(name: "ParseObjC", targets: ["Parse"]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change and is this a breaking change?
It seems a bit odd to name the core module Parse
, as the term "Parse" is arguably referring to the whole SDK. To me is makes more sense to name the core module ParseCore
to indicate that it's the basis that is always needed, while others are optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, yes it is an important change because in SDK target has the name "Parse". When we set "ParseCore" it causes an error downloading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand. Why can't we keep the name ParseCore
? I mean this is only an issue for the LiveQuery module, right? Otherwise we make a breaking change because developers changed their imports from Parse
to ParseCore
with the introduction of SPM, and now we are changing it back to Parse
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtrezza This is my first time using SPM and I didn't know you had to do import ParseCore;
. I can try to revert it back and fix the downloading error. @vazarkevych Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vazarkevych Could you take a look at this so we can close this PR? It seems to be the last remaining issue here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtrezza Can you commit 1 example of tedious work? I'll finish the rest. I don't quite understand what needs to be changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #1712 (comment). The module name ParseCore
shouldn't change, just because we're adding the LiveQuery module in this PR. Otherwise this PR becomes a breaking change, but it's just a feature addition.
So I'd simply remove the renaming and then go into the LiveQuery module and make sure it uses the ParseCore
reference. It seems the issue is that the LiveQuery module is looking for the Parse module by using its old Parse
reference instead of the ParseCore
reference in this repository. To me this looks like a simple find/replace task and some manual correction.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dplewis Do you want to take a look at this, we could add an additional bounty.
@mtrezza @vazarkevych @VolodyaNazarkevych This is ready for review. TODO: #1712 (review) |
Amazing! @VolodyaNazarkevych Could you please take a look at this and test this module out in a project? |
# [2.3.0](2.2.0...2.3.0) (2023-06-08) ### Features * Add LiveQuery module to SDK; this deprecates the separate [Parse LiveQuery SDK](https://github.com/parse-community/ParseLiveQuery-iOS-OSX) ([#1712](#1712)) ([154da34](154da34))
🎉 This change has been released in version 2.3.0 |
New Pull Request Checklist
Issue
Closes: #1714
TODOs before merging